Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat:add tls support fot memcached #5471

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat:add tls support fot memcached #5471

wants to merge 8 commits into from
+230 −43

Conversation

ozewr
Copy link

@ozewr ozewr commented Dec 27, 2024

Which issue does this PR close?

Closes #5419.

Rationale for this change

see #5419

What changes are included in this PR?

Modified the opendal::services::Memcached to support TLS connections.

Are there any user-facing changes?

Users can enable TLS using .tls() and provide the CA file using .cafile().
example:

let memcached = Memcached::default()
        .endpoint(r#"tcp://example.app.local:11211"#)
        .tls(true)
        .cafile(path);

@ozewr ozewr requested a review from Xuanwo as a code owner December 27, 2024 14:20
Xuanwo
Xuanwo reviewed Dec 27, 2024
View reviewed changes
core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/config.rs Outdated Show resolved Hide resolved
core/src/services/memcached/config.rs Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/binary.rs Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
Xuanwo
Xuanwo reviewed Dec 30, 2024
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like to add a test for memcached with tls enabled?

core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
@ozewr
Copy link
Author

ozewr commented Dec 31, 2024

Would you like to add a test for memcached with tls enabled?

No problem, I will complete it later.

@ozewr ozewr requested a review from PsiACE as a code owner January 2, 2025 10:07
@ozewr
Copy link
Author

ozewr commented Jan 2, 2025 •
edited
Loading

Does the Memcached service require any special configuration? During testing, I found many incorrect test results, and some tests occasionally pass while failing at other times during repeated tests. @Xuanwo
Here is my Memcached service configuration:

memcached --protocol=auto -p 11212 --enable-ssl -o ssl_chain_cert=./server.crt,ssl_key=./server.key,ssl_verify_mode=2,ssl_ca_cert=./client_ca/client_ca.crt

@Xuanwo Xuanwo mentioned this pull request Jan 2, 2025
Open
4 tasks
@ozewr ozewr requested a review from Xuanwo January 3, 2025 03:03
Xuanwo
Xuanwo reviewed Jan 3, 2025
View reviewed changes
core/Cargo.toml Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated
@@ -126,6 +148,22 @@ impl Builder for MemcachedBuilder {
.with_context("endpoint", &endpoint),
);
};
if self.config.enable_tls {
rustls::crypto::aws_lc_rs::default_provider()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned about this. I don't want to make decisions for users when it comes to choosing a crypto provider. Let's use ring as default for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a new boolean field? If the user selects true, they will need to provide it themselves.

core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated
let mut root_cert_store = rustls::RootCertStore::empty();

let native_certs = rustls_native_certs::load_native_certs();
if native_certs.errors.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't care about those errors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean just use .expect() or .unwrap()?

core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/backend.rs Outdated Show resolved Hide resolved
core/src/services/memcached/binary.rs Outdated Show resolved Hide resolved
@ozewr ozewr requested a review from Xuanwo January 3, 2025 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PsiACE PsiACE Awaiting requested review from PsiACE PsiACE is a code owner

@Xuanwo Xuanwo Awaiting requested review from Xuanwo Xuanwo is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
releases-note/feat
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

new feature: Memcached In Transit Encryption (TLS) support
2 participants
@ozewr @Xuanwo